-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat!: implement ADR 21: Subspace specific custom fee tokens #1138
Conversation
3d9715a
to
92e4ff9
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1138 +/- ##
==========================================
- Coverage 80.77% 80.32% -0.46%
==========================================
Files 193 190 -3
Lines 17159 16825 -334
==========================================
- Hits 13861 13515 -346
- Misses 2701 2716 +15
+ Partials 597 594 -3
☔ View full report in Codecov by Sentry. |
8aa800d
to
828775f
Compare
@RiccardoM @manu0466 Rebased |
28719f5
to
5f5e86e
Compare
5f5e86e
to
533416b
Compare
x/subspaces/ante/tx_checker.go
Outdated
newMinPrices := MergeMinPrices(ctx.MinGasPrices(), sdk.NewDecCoinsFromCoins(subspace.AllowedFeeTokens...)) | ||
newCtx := ctx.WithMinGasPrices(newMinPrices) | ||
return txFeeChecker(newCtx, tx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super familiar with on-chain gas prices, but I thought they were treated using the and
logic. So if I have a min gas price of 0.01udsm,0.01ustar
then to properly broadcast my transaction I have to pay fees in both tokens, and not either one of the two. Do you have any link for this to be checked out that we can paste here to remember this in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are treated as or
, so one of tokens reaches the min fees can broadcast properly, say, to send the message with 100 gas via the node with 0.01udsm, 0.01ustar
min gas prices, 1udsm
or 1ustar
can pass the check.
Here is the reference:
https://github.com/cosmos/cosmos-sdk/blob/release/v0.47.x/x/auth/ante/validator_tx_fee.go#L38
x/subspaces/types/models.go
Outdated
// Set allowed fee tokens without validating it. | ||
// Before storing the updated subspace, a validation with Validate() should | ||
// be performed. | ||
func (sub Subspace) SetAllowedFeeTokens(feeTokens sdk.Coins) Subspace { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a big fan of this approach. Instead I think it would be better to:
- Add another param inside the
NewSubspace
function to specify the fee tokens when creating the subspace. This is going to be useful when exporting/importing the genesis as well as inside tests. - Rename this method
Update
as we have done with thePost
andAttachment
type, to keep everything consistent.
Also, I'm doubting whether AllowedFeeTokens
is the proper name. I think AdditionalFeeTokens
might be better instead. The name AllowedFeeTokens
might make people think that if I specify 0.01ustar
then the star
token is going to be then only one allowed, while the udsm
should always be allowed anyway. Using AdditionalFeeTokens
might make everything more easy to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I use the SetAllowedFeeTokens
since adding another param inside the NewSubspace
will break hundreds of existing test codes, but yeah, it is better to have an additional field.
strongly agree with AdditionalFeeTokens
is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally, I used sed
to add the new parameter quickly:
#!/bin/bash
dir="./x"
find "$dir" -type f -name "*.go" | while read -r file; do
sed -i -E ':a;N;$!ba;s/NewSubspace\([^)]*time.UTC,/&\nnil,/g' "$file"
echo "$file"
done
Is any other tools to make modifying codes quickly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly don't know of any other tool that can easily update the code. I would have done it by hand 😓 Great job using sed
💯
f911e1e
to
3437a0b
Compare
3e15154
to
cecf756
Compare
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
This reverts commit 3437a0b.
cecf756
to
e674ee7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job 💯
Description
This PR implements ADR-21: Subspace specific custom fee tokens feature.
Closes: DCD-312
Depends-On: #1135Depends-On: #1139Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change